feat: client cache for configurable list of gvks - #1042
Open
SoWieMarkus wants to merge 14 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe pull request adds a configurable in-process client-cache overlay with informer eviction and TTL cleanup. The manager registers the cache and routes controller, reader, reconciler, monitor, and task operations through it while retaining the multicluster client for setup. ChangesClient cache integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant CachingClient
participant Controllers
participant MulticlusterClient
participant Informers
Manager->>CachingClient: New(inner client, scheme, config)
Manager->>Manager: Register CachingClient
Controllers->>CachingClient: Get, List, Create, Update, Patch, Delete
CachingClient->>MulticlusterClient: Delegate client operation
CachingClient-->>Controllers: Return merged overlay or inner result
CachingClient->>MulticlusterClient: GetInformersForKind(ctx, object)
MulticlusterClient->>Informers: Retrieve configured informers
Informers-->>CachingClient: Resource events
CachingClient->>CachingClient: Evict matching overlay entries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mblos
marked this pull request as ready for review
July 16, 2026 08:11
mblos
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
July 16, 2026 08:11
mblos
reviewed
Jul 16, 2026
mblos
left a comment
mblos
left a comment
Collaborator
There was a problem hiding this comment.
Very nice work :) some first comments (still not completed with reviewing)
PhilippMatthes
requested changes
Jul 20, 2026
PhilippMatthes
left a comment
PhilippMatthes
left a comment
Member
There was a problem hiding this comment.
Thank you for incorporating my feedback from #1015 -- especially, the informer-cache idea and reusing metav1.Duration! I've copied over some thoughts and questions and had some new ones along the way. Thanks for considering my feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/manager/main.go (1)
520-531: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute the inflight reservation controller through the caching client.
inflight.Controller.SetupWithManagerchecks thatc.Clientis*multicluster.Client, soController{Client: multiclusterClient}bypassescachingClient. Sinceclientcacheconfig enablescortex.cloud/v1alpha1/Reservation, the inflight reservation reconciler can observe informer-laggedReservationreads while writes go directly throughmulticlusterClient; assign an inner caching client instead of a bare multicluster client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/manager/main.go` around lines 520 - 531, Update the inflight controller initialization in the controller setup block to pass the inner caching client configured for Reservation resources, rather than the bare multiclusterClient. Preserve the existing VMClient and SetupWithManager flow, and ensure the assigned Client remains compatible with inflight.Controller’s expected multicluster client type.
🧹 Nitpick comments (2)
pkg/clientcache/cache.go (1)
199-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
fieldSetLockedkeeps only the first value per indexed field.
fields.Setmaps one value per field, so an overlay-only object with several index values matches only one of them. The controller-runtime informer index matches any of the values. AMatchingFieldsquery can therefore miss an overlay-only object. Match the selector against each indexed value instead of building a singlefields.Set.♻️ Proposed change to match any indexed value
- if lo.FieldSelector != nil && !lo.FieldSelector.Empty() { - set := o.fieldSetLocked(gvk, obj) - if !lo.FieldSelector.Matches(set) { - return false - } - } + if lo.FieldSelector != nil && !lo.FieldSelector.Empty() { + if !o.matchesFieldsLocked(gvk, obj, lo.FieldSelector) { + return false + } + }// matchesFieldsLocked reports whether any combination of indexed values for // the GVK satisfies the selector. Callers must hold at least the read lock. func (o *overlay) matchesFieldsLocked(gvk schema.GroupVersionKind, obj client.Object, sel fields.Selector) bool { for _, req := range sel.Requirements() { fn, ok := o.indexers[gvk][req.Field] if !ok { return false } if !slices.Contains(fn(obj), req.Value) { return false } } return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clientcache/cache.go` around lines 199 - 212, Replace the single-value fieldSetLocked approach with selector matching that evaluates every indexed value for each requirement. Add or update an overlay method such as matchesFieldsLocked to resolve each requested field, require all selector requirements to pass, and accept a requirement when any value returned by its indexer matches; preserve failure for unregistered fields.internal/scheduling/nova/hypervisor_overcommit_controller.go (1)
220-230: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemoved client validation leaves both the setup path and its test without a deterministic failure.
SetupWithManagerno longer validates the client it builds watches with, and the test that covered that validation now passesniland relies on config loading failing first.
internal/scheduling/nova/hypervisor_overcommit_controller.go#L220-L230: add an explicitmcl == nilguard that returnserrors.New("multicluster client must not be nil")before the config load.internal/scheduling/nova/hypervisor_overcommit_controller_test.go#L710-L734: rename the test to describe the nil-client case and assert the specific returned error instead of accepting any error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduling/nova/hypervisor_overcommit_controller.go` around lines 220 - 230, The SetupWithManager path must deterministically reject a nil multicluster client before loading configuration. In internal/scheduling/nova/hypervisor_overcommit_controller.go:220-230, add the specified nil guard returning errors.New("multicluster client must not be nil"); in internal/scheduling/nova/hypervisor_overcommit_controller_test.go:710-734, rename the test to describe the nil-client case and assert that exact error instead of accepting any error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/manager/main.go`:
- Around line 400-414: Update the index registration flow to call IndexField on
cachingClient rather than mcl, ensuring registrations populate overlay.indexers
and MatchingFields includes overlay-only objects. Locate the existing index
setup calls and route each through the clientcache wrapper while preserving
their current fields and index functions.
In `@pkg/clientcache/client.go`:
- Around line 185-189: Update the live overlay handling in Get() to deep-copy
e.obj via DeepCopyObject() before passing it to scheme.Convert, then convert the
copied object into obj. Preserve the existing conversion error propagation and
successful return behavior, ensuring callers cannot mutate the cached overlay
entry through shared maps, slices, or metadata.
In `@pkg/clientcache/runnable.go`:
- Around line 20-55: Add NeedLeaderElection() bool to CachingClient, returning
false, so its Start lifecycle—including eviction handlers and TTL cleanup—runs
on every replica regardless of leader election. Ensure AsRunnable() exposes this
method through the manager.Runnable implementation.
---
Outside diff comments:
In `@cmd/manager/main.go`:
- Around line 520-531: Update the inflight controller initialization in the
controller setup block to pass the inner caching client configured for
Reservation resources, rather than the bare multiclusterClient. Preserve the
existing VMClient and SetupWithManager flow, and ensure the assigned Client
remains compatible with inflight.Controller’s expected multicluster client type.
---
Nitpick comments:
In `@internal/scheduling/nova/hypervisor_overcommit_controller.go`:
- Around line 220-230: The SetupWithManager path must deterministically reject a
nil multicluster client before loading configuration. In
internal/scheduling/nova/hypervisor_overcommit_controller.go:220-230, add the
specified nil guard returning errors.New("multicluster client must not be nil");
in internal/scheduling/nova/hypervisor_overcommit_controller_test.go:710-734,
rename the test to describe the nil-client case and assert that exact error
instead of accepting any error.
In `@pkg/clientcache/cache.go`:
- Around line 199-212: Replace the single-value fieldSetLocked approach with
selector matching that evaluates every indexed value for each requirement. Add
or update an overlay method such as matchesFieldsLocked to resolve each
requested field, require all selector requirements to pass, and accept a
requirement when any value returned by its indexer matches; preserve failure for
unregistered fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 404cddc7-8f51-4e05-a9bc-9fb4f2791c3b
📒 Files selected for processing (12)
cmd/manager/main.gohelm/bundles/cortex-nova/values.yamlinternal/scheduling/nova/hypervisor_overcommit_controller.gointernal/scheduling/nova/hypervisor_overcommit_controller_test.gopkg/clientcache/cache.gopkg/clientcache/cache_test.gopkg/clientcache/client.gopkg/clientcache/client_test.gopkg/clientcache/config.gopkg/clientcache/interfaces.gopkg/clientcache/runnable.gopkg/multicluster/client.go
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
…erlay stale reads during concurrent updates
…ject in Get method
…nagement across replicas
SoWieMarkus
force-pushed
the
feat/client-cache
branch
from
August 7, 2026 08:29
18796b2 to
2c201bc
Compare
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
SoWieMarkus
force-pushed
the
feat/client-cache
branch
from
August 10, 2026 07:22
5101ce2 to
8f93636
Compare
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
Contributor
Test Coverage ReportTest Coverage 📊: 70.7% |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/clientcache/client.go (1)
152-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider collapsing the duplicate inner-client fields.
CachingClientembedsclient.Clientand also storesinner Client.Newassigns the same value to both. The localClientinterface already embedsclient.Client, so embeddingClientdirectly gives both the delegation methods andGetInformersForKind. This removes the risk that the two fields diverge.♻️ Proposed change
type CachingClient struct { - client.Client // inner client, used for delegation + Client // inner client, used for delegation and informer access - inner Client scheme *runtime.SchemeThen drop the
inner: inner,line inNewand replacec.inner.GetInformersForKindinpkg/clientcache/runnable.gowithc.GetInformersForKind.Also applies to: 184-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clientcache/client.go` around lines 152 - 155, Collapse the duplicate client fields in CachingClient by embedding the local Client interface instead of client.Client and removing the separate inner Client field. Update New to stop assigning inner, and change runnable.go’s c.inner.GetInformersForKind call to c.GetInformersForKind while preserving existing delegation behavior.pkg/clientcache/client_test.go (1)
127-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
deleteAllOfErris never exercised.No test sets
deleteAllOfErr, so the field on Line 127 and theDeleteAllOfoverride on Lines 160-165 are unused. Either add aDeleteAllOfcase toTestWriteErrorLeavesOverlayUntouchedor remove both. A failure case is useful here, becauseDeleteAllOfinpkg/clientcache/client.gomust leave every overlay entry untouched when the inner call fails.As per coding guidelines: "Test files should be short and contain only necessary test cases".
♻️ Proposed test case
{ name: "delete", seed: true, mkClient: func(inner *fakeClient) Client { return &errClient{Client: inner, deleteErr: sentinel} }, op: func(c *CachingClient, r *v1alpha1.Reservation) error { return c.Delete(context.Background(), r) }, }, + { + name: "deleteallof", + seed: true, + mkClient: func(inner *fakeClient) Client { return &errClient{Client: inner, deleteAllOfErr: sentinel} }, + op: func(c *CachingClient, r *v1alpha1.Reservation) error { + return c.DeleteAllOf(context.Background(), &v1alpha1.Reservation{}) + }, + },Also applies to: 160-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/clientcache/client_test.go` at line 127, Remove the unused deleteAllOfErr field and its DeleteAllOf override from the test mock, unless you add a meaningful DeleteAllOf failure case to TestWriteErrorLeavesOverlayUntouched that verifies overlays remain unchanged when the inner call fails.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/clientcache/client_test.go`:
- Around line 438-462: Update TestDeleteAllOf after the DeleteAllOf call to
recreate res-dao-1 directly in the inner client, using the same approach as
TestTombstone, before calling c.Get. Keep the existing assertions so the
NotFound result verifies the cache tombstone rather than deletion from the
underlying client.
In `@pkg/clientcache/client.go`:
- Around line 512-537: Protect DeleteAllOf and single-object write paths with a
package-level sync.RWMutex: add a bulkLock field to CachingClient, acquire
bulkLock.RLock() alongside the existing writeLocks lock in Create, Update,
Patch, and Delete, and acquire bulkLock.Lock() across the inner DeleteAllOf call
and overlay mutation in DeleteAllOf. Preserve the existing per-object locking
and cleanup behavior.
- Around line 425-438: Replace the single-value fieldSetLocked matching flow
with per-requirement matching, using a matchesFieldSelectorLocked helper. For
each selector requirement, evaluate every value returned by the registered
indexer and accept the object when any value satisfies that requirement; require
all requirements to match and return false when the indexer is missing or no
value matches. Remove the first-value-only behavior and update callers to use
the new helper.
---
Nitpick comments:
In `@pkg/clientcache/client_test.go`:
- Line 127: Remove the unused deleteAllOfErr field and its DeleteAllOf override
from the test mock, unless you add a meaningful DeleteAllOf failure case to
TestWriteErrorLeavesOverlayUntouched that verifies overlays remain unchanged
when the inner call fails.
In `@pkg/clientcache/client.go`:
- Around line 152-155: Collapse the duplicate client fields in CachingClient by
embedding the local Client interface instead of client.Client and removing the
separate inner Client field. Update New to stop assigning inner, and change
runnable.go’s c.inner.GetInformersForKind call to c.GetInformersForKind while
preserving existing delegation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 780c7525-3d2d-44d8-b1c1-bca1f04c73fa
📒 Files selected for processing (7)
cmd/manager/main.gointernal/scheduling/reservations/inflight/controller.gointernal/scheduling/reservations/inflight/controller_test.gopkg/clientcache/client.gopkg/clientcache/client_test.gopkg/clientcache/interfaces.gopkg/clientcache/runnable.go
💤 Files with no reviewable changes (1)
- internal/scheduling/reservations/inflight/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/clientcache/runnable.go
- cmd/manager/main.go
Comment on lines
+438
to
+462
| func TestDeleteAllOf(t *testing.T) { | ||
| r1 := newReservation("res-dao-1", "az-1", "1") | ||
| r1.Labels = map[string]string{"zone": "a"} | ||
| r2 := newReservation("res-dao-2", "az-2", "1") | ||
| r2.Labels = map[string]string{"zone": "b"} | ||
| inner := newTestClient(t, r1, r2) | ||
| c := newCaching(t, inner) | ||
|
|
||
| // Populate overlay for both so we can verify tombstoning. | ||
| c.upsert(reservationGVK(), r1) | ||
| c.upsert(reservationGVK(), r2) | ||
|
|
||
| // DeleteAllOf with a label selector — only r1 should be tombstoned. | ||
| if err := c.DeleteAllOf(context.Background(), &v1alpha1.Reservation{}, client.MatchingLabels{"zone": "a"}); err != nil { | ||
| t.Fatalf("DeleteAllOf: %v", err) | ||
| } | ||
|
|
||
| var got v1alpha1.Reservation | ||
| if err := c.Get(context.Background(), types.NamespacedName{Name: "res-dao-1"}, &got); !apierrors.IsNotFound(err) { | ||
| t.Fatalf("expected NotFound for tombstoned res-dao-1, got %v", err) | ||
| } | ||
| if err := c.Get(context.Background(), types.NamespacedName{Name: "res-dao-2"}, &got); err != nil { | ||
| t.Fatalf("res-dao-2 should still be visible, got %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
TestDeleteAllOf does not isolate the tombstone behaviour.
r1 exists in the inner fake client, and c.DeleteAllOf deletes it there as well. The Get on Line 456 therefore returns NotFound from the inner client regardless of the overlay. The test passes even if the tombstone loop in DeleteAllOf is removed.
Re-create res-dao-1 directly in inner after the DeleteAllOf call, the same way TestTombstone does on Line 425. The NotFound then proves the tombstone is applied.
💚 Proposed change
if err := c.DeleteAllOf(context.Background(), &v1alpha1.Reservation{}, client.MatchingLabels{"zone": "a"}); err != nil {
t.Fatalf("DeleteAllOf: %v", err)
}
+ // Re-create directly in inner to simulate informer lag; only the tombstone
+ // can hide the object now.
+ if err := inner.Create(context.Background(), newReservation("res-dao-1", "az-1", "")); err != nil {
+ t.Fatalf("re-create inner: %v", err)
+ }
var got v1alpha1.Reservation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestDeleteAllOf(t *testing.T) { | |
| r1 := newReservation("res-dao-1", "az-1", "1") | |
| r1.Labels = map[string]string{"zone": "a"} | |
| r2 := newReservation("res-dao-2", "az-2", "1") | |
| r2.Labels = map[string]string{"zone": "b"} | |
| inner := newTestClient(t, r1, r2) | |
| c := newCaching(t, inner) | |
| // Populate overlay for both so we can verify tombstoning. | |
| c.upsert(reservationGVK(), r1) | |
| c.upsert(reservationGVK(), r2) | |
| // DeleteAllOf with a label selector — only r1 should be tombstoned. | |
| if err := c.DeleteAllOf(context.Background(), &v1alpha1.Reservation{}, client.MatchingLabels{"zone": "a"}); err != nil { | |
| t.Fatalf("DeleteAllOf: %v", err) | |
| } | |
| var got v1alpha1.Reservation | |
| if err := c.Get(context.Background(), types.NamespacedName{Name: "res-dao-1"}, &got); !apierrors.IsNotFound(err) { | |
| t.Fatalf("expected NotFound for tombstoned res-dao-1, got %v", err) | |
| } | |
| if err := c.Get(context.Background(), types.NamespacedName{Name: "res-dao-2"}, &got); err != nil { | |
| t.Fatalf("res-dao-2 should still be visible, got %v", err) | |
| } | |
| } | |
| func TestDeleteAllOf(t *testing.T) { | |
| r1 := newReservation("res-dao-1", "az-1", "1") | |
| r1.Labels = map[string]string{"zone": "a"} | |
| r2 := newReservation("res-dao-2", "az-2", "1") | |
| r2.Labels = map[string]string{"zone": "b"} | |
| inner := newTestClient(t, r1, r2) | |
| c := newCaching(t, inner) | |
| // Populate overlay for both so we can verify tombstoning. | |
| c.upsert(reservationGVK(), r1) | |
| c.upsert(reservationGVK(), r2) | |
| // DeleteAllOf with a label selector — only r1 should be tombstoned. | |
| if err := c.DeleteAllOf(context.Background(), &v1alpha1.Reservation{}, client.MatchingLabels{"zone": "a"}); err != nil { | |
| t.Fatalf("DeleteAllOf: %v", err) | |
| } | |
| // Re-create directly in inner to simulate informer lag; only the tombstone | |
| // can hide the object now. | |
| if err := inner.Create(context.Background(), newReservation("res-dao-1", "az-1", "")); err != nil { | |
| t.Fatalf("re-create inner: %v", err) | |
| } | |
| var got v1alpha1.Reservation | |
| if err := c.Get(context.Background(), types.NamespacedName{Name: "res-dao-1"}, &got); !apierrors.IsNotFound(err) { | |
| t.Fatalf("expected NotFound for tombstoned res-dao-1, got %v", err) | |
| } | |
| if err := c.Get(context.Background(), types.NamespacedName{Name: "res-dao-2"}, &got); err != nil { | |
| t.Fatalf("res-dao-2 should still be visible, got %v", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/clientcache/client_test.go` around lines 438 - 462, Update
TestDeleteAllOf after the DeleteAllOf call to recreate res-dao-1 directly in the
inner client, using the same approach as TestTombstone, before calling c.Get.
Keep the existing assertions so the NotFound result verifies the cache tombstone
rather than deletion from the underlying client.
Comment on lines
+425
to
+438
| // fieldSetLocked builds a fields.Set for obj using the registered IndexerFuncs | ||
| // for the GVK. Callers must hold at least the read lock. | ||
| func (c *CachingClient) fieldSetLocked(gvk schema.GroupVersionKind, obj client.Object) fields.Set { | ||
| set := fields.Set{} | ||
| for field, fn := range c.indexers[gvk] { | ||
| for _, v := range fn(obj) { | ||
| // A field selector matches a single value; take the first indexed | ||
| // value for the field (mirrors controller-runtime cache behaviour). | ||
| set[field] = v | ||
| break | ||
| } | ||
| } | ||
| return set | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fieldSetLocked drops all but the first indexed value.
An IndexerFunc can return several values. fields.Set is a map[string]string, so only one value survives, and a MatchingFields query on any other value returns no match for overlay entries. The controller-runtime cache matches if any indexed value equals the requested value, so the comment on Line 431 does not describe the real behaviour.
The registered index in internal/scheduling/reservations/inflight/controller.go returns at most one value today, so this is not reachable yet. Consider matching per-field against every indexed value instead of building a single fields.Set.
🐛 Proposed change
- if lo.FieldSelector != nil && !lo.FieldSelector.Empty() {
- set := c.fieldSetLocked(gvk, obj)
- if !lo.FieldSelector.Matches(set) {
- return false
- }
- }
+ if lo.FieldSelector != nil && !lo.FieldSelector.Empty() {
+ if !c.matchesFieldSelectorLocked(gvk, obj, lo.FieldSelector) {
+ return false
+ }
+ }// matchesFieldSelectorLocked reports whether obj satisfies every requirement of
// sel. A field matches when any value produced by its IndexerFunc matches.
// Callers must hold at least the read lock.
func (c *CachingClient) matchesFieldSelectorLocked(gvk schema.GroupVersionKind, obj client.Object, sel fields.Selector) bool {
for _, req := range sel.Requirements() {
fn, ok := c.indexers[gvk][req.Field]
if !ok {
return false
}
matched := false
for _, v := range fn(obj) {
if sel.Matches(fields.Set{req.Field: v}) {
matched = true
break
}
}
if !matched {
return false
}
}
return true
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/clientcache/client.go` around lines 425 - 438, Replace the single-value
fieldSetLocked matching flow with per-requirement matching, using a
matchesFieldSelectorLocked helper. For each selector requirement, evaluate every
value returned by the registered indexer and accept the object when any value
satisfies that requirement; require all requirements to match and return false
when the indexer is missing or no value matches. Remove the first-value-only
behavior and update callers to use the new helper.
Comment on lines
+512
to
+537
| func (c *CachingClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error { | ||
| gvk, cached := c.gvkFor(obj) | ||
| if !cached { | ||
| return c.Client.DeleteAllOf(ctx, obj, opts...) | ||
| } | ||
| if err := c.Client.DeleteAllOf(ctx, obj, opts...); err != nil { | ||
| return err | ||
| } | ||
| dao := &client.DeleteAllOfOptions{} | ||
| dao.ApplyOptions(opts) | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| for key, e := range c.byGVK[gvk] { | ||
| if !c.matchesLocked(gvk, e.obj, &dao.ListOptions) { | ||
| continue | ||
| } | ||
| c.byGVK[gvk][key] = &entry{ | ||
| obj: e.obj, | ||
| uid: e.uid, | ||
| resourceVersion: e.resourceVersion, | ||
| deleted: true, | ||
| expiresAt: time.Now().Add(c.ttl), | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
DeleteAllOf does not take the per-object write lock.
Create, Update, Patch, and Delete hold writeLocks across the inner call and the overlay mutation. DeleteAllOf skips that lock. A concurrent Update on a matching object can commit to the apiserver first, then DeleteAllOf deletes it, and then the Update goroutine runs c.upsert and re-adds a live overlay entry for an object that no longer exists. Reads then serve the deleted object until the TTL expires or an informer delete event arrives. This is the same ordering hazard that writeLocks and TestConcurrentUpdatesOverlayNotBehind were added to prevent.
DeleteAllOf cannot know the affected keys before the inner call, so a per-object lock does not fit. A package-level sync.RWMutex works: per-object writers take it in read mode, DeleteAllOf takes it in write mode.
🛡️ Sketch of a guard
type CachingClient struct {
...
writeLocks *keyedMutex
+ // bulkLock excludes DeleteAllOf from all single-object writes. Single-object
+ // writers hold it in read mode; DeleteAllOf holds it in write mode.
+ bulkLock sync.RWMutex
} func (c *CachingClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ...client.DeleteAllOfOption) error {
gvk, cached := c.gvkFor(obj)
if !cached {
return c.Client.DeleteAllOf(ctx, obj, opts...)
}
+ c.bulkLock.Lock()
+ defer c.bulkLock.Unlock()
if err := c.Client.DeleteAllOf(ctx, obj, opts...); err != nil {Each single-object write path then adds c.bulkLock.RLock() / defer c.bulkLock.RUnlock() next to the existing writeLocks.lock call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/clientcache/client.go` around lines 512 - 537, Protect DeleteAllOf and
single-object write paths with a package-level sync.RWMutex: add a bulkLock
field to CachingClient, acquire bulkLock.RLock() alongside the existing
writeLocks lock in Create, Update, Patch, and Delete, and acquire
bulkLock.Lock() across the inner DeleteAllOf call and overlay mutation in
DeleteAllOf. Preserve the existing per-object locking and cleanup behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
flowchart TB Ctrl["Controller"] CC["CachingClient\nOverlay: {namespace, name} → entry\n• live entry (write pending)\n• tombstone (delete pending)"] MCL["multicluster.Client\n(Routing)"] Home[("Home Cluster\nfoo/my-vm")] R1[("Remote A\nfoo/my-vm ⚠ same key!")] R2[("Remote B")] Ctrl -->|"Write → live entry\nDelete → tombstone"| CC CC -->|"Get: tombstone → NotFound\nGet: live entry → overlay wins"| Ctrl CC --> MCL MCL --> Home MCL --> R1 MCL --> R2 Home -. "Informer Events → evict entry/tombstone" .-> CC R1 -. "Informer Events → evict entry/tombstone" .-> CC R2 -. "Informer Events → evict entry/tombstone" .-> CC